Memory Leak Logs #1159
Merged
Memory Leak Logs #1159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Chaurasiya, Payal <[email protected]>
rahulga1
approved these changes
Nov 19, 2024
teoparvanov
approved these changes
Nov 19, 2024
Signed-off-by: Chaurasiya, Payal <[email protected]>
Signed-off-by: Chaurasiya, Payal <[email protected]>
Signed-off-by: Chaurasiya, Payal <[email protected]>
Signed-off-by: Chaurasiya, Payal <[email protected]>
Signed-off-by: Chaurasiya, Payal <[email protected]>
MasterSkepticista
requested changes
Nov 20, 2024
There was a problem hiding this comment.
@payalcha Per our last discussion, I'm not in favor of this PR for the following reasons:
- There is evidence of memory growth from internal experiments, which means we need to identify the source of the growth/leak to disposition it. If the investigation suggests changes to
openflto fix the issue, that is a great PR to have. - In a situation where the leak/growth is due to an underlying framework (pandas, tf, torch etc.), or it falls outside of
openflpurview, it needs to be tracked in the respective framework repository.
That said, if the overall decision is to still add this telemetry functionality, I suggest you use decorators over functions that you want to log memory for, within your CI tests. All helper functions can live within tests/. I don't think framework source code (anything within openfl/) is the right place for it, because users are neither required nor encouraged to use it in their usual experiments.
Signed-off-by: Chaurasiya, Payal <[email protected]>
Signed-off-by: Chaurasiya, Payal <[email protected]>
MasterSkepticista
requested changes
Nov 21, 2024
Signed-off-by: Chaurasiya, Payal <[email protected]>
Signed-off-by: Chaurasiya, Payal <[email protected]>
MasterSkepticista
approved these changes
Nov 21, 2024
Signed-off-by: Chaurasiya, Payal <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewed PR - #1155 (Had to close as some unsigned commits were there)
Openfl component change
Add memory leak logs for aggregator and collaborator, if log_memory_usage flag can be set for ubuntu.
Changes -
Output -
agg_mem_details.json - details of aggregator in json format
_mem_details.json - details of aggregator in json format
Structure -
{
"round_number": 0,
"metric_origin": "collaborator1",
"process_memory": 1155.19,
"virtual_memory": {
},
"swap_memory": {
}
}
Test changes
federation_helper.py - _verify_completion_for_participant - added timeout on the basis of num_rounds.